Project management updates #16
Conversation
| // It's actually important to run this async, | ||
| // because createNewProject() will not resolve | ||
| // its Promise until a path is selected, | ||
| // allowing the user to cancel the open dialog |
There was a problem hiding this comment.
And this proves out one of the things I like about staging our processes from a webview dialog. In this case, the user can cancel the open file dialog, or with the "open existing" use case, can select an invalid folder, and they can just try again without losing their state, by clicking the respective "launch" button in the webview again. I think that's a better way to go than anchoring with the controls that can so easily be dismissed from the UX.
| return new Promise(async (resolve) => { | ||
| this.projectConfigurationProcessor.getProjectManagementChoice( | ||
| (panel) => { | ||
| // It's actually important to run this async, because |
| const folderUri = | ||
| await this.projectConfigurationProcessor.getProjectFolderPath(); | ||
| if (!folderUri || folderUri.length === 0) { | ||
| return; |
There was a problem hiding this comment.
We explicitly don't want to reject. This is what allows for the "recoverable" case: the Promise will not be fulfilled one way or another, until a proper value is returned, allowing the user to cancel the folder picker any number of times and still continue the workflow.
There was a problem hiding this comment.
I'm adding comments to that effect in the relevant areas. It's a good place to add clarity.
There was a problem hiding this comment.
Ah, awesome! That makes sense now. Thanks!
| const folderUri = | ||
| await this.projectConfigurationProcessor.getProjectFolderPath(); | ||
| if (!folderUri || folderUri.length === 0) { | ||
| return; |
|
|
||
| // Is this the Offline Starter Kit repo? | ||
| try { | ||
| const oskInitialCommit = |
There was a problem hiding this comment.
Should we put this in constants somewhere perhaps, or at least in a static readonly?
| <body> | ||
| <p>Follow the prompts to configure the project.</p> | ||
| <p> | ||
| <strong>NOTE:</strong> |
There was a problem hiding this comment.
Ugggh, I was making a joke about using blink tags, but git removed the <blink/>. Sorry!
There was a problem hiding this comment.
Haha you know I had a feeling that's what you meant! But I was like, "But tags? ¯\(ツ)/¯ " We were on the same page all along.
src/test/TestHelper.ts
Outdated
| projectDirStats = await stat(projectDir); | ||
| } catch (err) { | ||
| return reject( | ||
| `Project dir '${projectDir}' does not exist or is inaccessible.` |
There was a problem hiding this comment.
Assuming these don't need to be l10n-ized?
There was a problem hiding this comment.
They're just for tests, so I didn't bother.
There was a problem hiding this comment.
Derp, I missed that it was in a test case. Thanks.
src/test/TestHelper.ts
Outdated
| if (!projectDirStats.isDirectory()) { | ||
| return reject(`Project dir '${projectDir}' is not a directory.`); | ||
| } | ||
| return rm(projectDir, { recursive: true, force: true }); |
There was a problem hiding this comment.
This is scary to me - is there any way this could be abused or broken and end up deleting someone's filesystem? Maybe we leave cleanup to end user?
There was a problem hiding this comment.
Ha, this is a test utility, so not a big deal. At first I thought it was part of the command. Ha. Still a bit scary to do a recursive delete on something unless it is absolutely necessary.
There was a problem hiding this comment.
I thought about that too, which is why I did all the stuff around validating the folder, as best I could. That said, they're just temp folders which will eventually get cleaned up anyway, so we could (ahem) remove the removal logic, if you all would prefer.
There was a problem hiding this comment.
I have an idea to split the difference: make a data structure that only allows the created folder to be removed. That way we can virtually guarantee that we're talking about a temp folder. I'll code that up, shouldn't take a minute.
| assert.equal(origCwd, process.cwd()); | ||
| removeTempProjectDir(projectFolderUri.fsPath); | ||
| } | ||
| }).timeout(10000); |
There was a problem hiding this comment.
Wondering if 10 seconds is long enough or if this can create flapper? Maybe 60secs?
There was a problem hiding this comment.
I figured I'd wait to see how it panned out in CI, but I'm totally open to upping it. Locally, it's just above the 2 sec default timeout, but you never know.
| 'git remote add origin https://github.com/salesforce/offline-app-developer-starter-kit.git' | ||
| ); | ||
| await CommonUtils.executeCommandAsync( | ||
| 'git fetch origin 99b1fa9377694beb7918580aab445a2e9981f611' |
There was a problem hiding this comment.
Should SHA actually be synced using package.json so that the code and the test can share?
There was a problem hiding this comment.
I can make it an accessible property in the code here, and then just import it in the test. That's a good idea.
There's still quite a bit of refactoring to go:- Split out messaging to resource file(s)- Testing, which will almost certainly require some refactoring to support it.So it's a draft, but it's a working code base for the different project bootstrap use cases.